BigQuery user-scoped OAuth creds - #287

tests/dialects/test_bigquery.py (2)

681-691: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider annotating the fixture for the secret scanner.

Betterleaks flags lines 682-689 as Google Application Default Credentials. The finding is a false positive: the values are placeholders and the dict only matches the authorized-user JSON shape. A suppression comment keeps the secret-scanning signal clean. The repository already applies this pattern in tests/test_engine_factory.py lines 31-33 with # NOSONAR(S2068).

🧹 Proposed annotation

 def _oauth_info(**overrides) -> dict:
-    info = {
+    info = {  # noqa: S106 — test fixture; placeholder OAuth grant, not real credentials
         "type": "authorized_user",
         "client_id": "cid.apps.googleusercontent.com",
         "client_secret": "csecret",
         "refresh_token": "rtok-alice",
         "token": "access-token-1",
         "token_uri": "https://oauth2.googleapis.com/token",
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/dialects/test_bigquery.py` around lines 681 - 691, Add the
repository-standard NOSONAR(S2068) suppression annotation to the _oauth_info
fixture, covering the placeholder authorized-user credential dictionary flagged
by Betterleaks. Keep the fixture values and behavior unchanged, following the
existing suppression pattern used elsewhere in the tests.

Source: Linters/SAST tools

---

795-854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a fingerprint test for a malformed OAuth payload.

credential_fingerprint runs on every cache-key computation through slayer/sql/engine_factory.py line 113-124. It must never raise, otherwise an invalid stored datasource breaks engine lookup instead of producing the clear build_engine error. _durable_oauth_material handles that with the JSONDecodeError fallback at slayer/sql/dialects/bigquery.py lines 114-115, but no test pins it.

🧪 Proposed test

def test_credential_fingerprint_tolerates_malformed_oauth_json() -> None:
    """The fingerprint feeds every cache-key lookup, so a bad grant must
    produce a digest rather than raise. build_engine reports the error."""
    ds = DatasourceConfig(
        name="bq", type="bigquery", oauth_credentials_json="not json at all",
    )
    assert BigqueryDialect().credential_fingerprint(ds)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/dialects/test_bigquery.py` around lines 795 - 854, Add a test alongside
the existing credential_fingerprint tests that constructs a Bigquery
DatasourceConfig with malformed oauth_credentials_json and asserts
BigqueryDialect().credential_fingerprint returns a non-empty fingerprint without
raising. Preserve the test’s focus on tolerant cache-key generation for invalid
OAuth payloads.

slayer/engine/query_engine.py (1)

94-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Use keyword arguments for cache-key calls.

• slayer/engine/query_engine.py#L94-L96: Call _engine_cache_key with datasource= and connection_string=.
• slayer/sql/engine_factory.py#L237-L237: Call _cache_key with datasource= and connection_string=.
• slayer/sql/engine_factory.py#L264-L265: Call _cache_key with datasource= and connection_string=.

As per coding guidelines, “Use keyword arguments for functions with more than one parameter.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/engine/query_engine.py` around lines 94 - 96, Update _engine_cache_key
in slayer/engine/query_engine.py:94-96 to pass datasource= and
connection_string= as keyword arguments. Update both _cache_key calls in
slayer/sql/engine_factory.py:237 and 264-265 the same way, preserving the
existing argument values.

Source: Coding guidelines

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

tests/test_engine_factory.py (3)

556-566: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider sharing the _oauth_ds helper.

tests/dialects/test_bigquery.py defines a helper with the same name and the same body shape. The two copies have already diverged: the BigQuery copy accepts additional keyword arguments such as quota_project_id, token, and expiry.

Move one implementation into a shared test helper or a conftest fixture so both files stay in step.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_engine_factory.py` around lines 556 - 566, The duplicated
_oauth_ds helper in tests/test_engine_factory.py and
tests/dialects/test_bigquery.py should be consolidated into one shared test
helper or conftest fixture. Move the implementation to the shared location,
preserve support for the BigQuery helper’s additional keyword arguments such as
quota_project_id, token, and expiry, and update both test files to reuse it.

---

291-337: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider an autouse fixture for cache reset.

Every test in this class calls engine_factory.reset_cache() at the start and at the end. The trailing call does not run when an assertion fails, so a failure leaves module-global cache state for later tests.

An autouse fixture that resets before and after each test removes the duplication and makes cleanup unconditional.

♻️ Proposed fixture

 class TestCacheBounding:
     """Per-identity keys make cache cardinality track *users*, not
     datasources, so the cache has to be bounded and evictions must actually
     release the pooled connections."""
 
+    `@pytest.fixture`(autouse=True)
+    def _clean_cache(self):
+        engine_factory.reset_cache()
+        yield
+        engine_factory.reset_cache()
+
     `@staticmethod`
     def _lite(n: int) -> DatasourceConfig:

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_engine_factory.py` around lines 291 - 337, Use an autouse fixture
for the test class that calls engine_factory.reset_cache() before each test and
guarantees a second reset during teardown. Remove the duplicated start/end
reset_cache() calls from test_cache_evicts_least_recently_used_over_limit,
test_reuse_refreshes_recency, test_eviction_disposes_the_engine, and
test_dispose_failure_does_not_break_caching.

---

287-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer the tmp_path fixture over fixed /tmp database paths. Four datasource helpers hard-code SQLite file names under /tmp. The names are fixed, so two concurrent runs or two users on the same host collide. get_engine does not open a connection in most of these tests, so nothing is written today, but any test that later executes a statement would create a world-readable file at a predictable path. Static analysis flags all four sites (hardcoded-tmp-file, CWE-377).

• tests/test_engine_factory.py#L287-L289: take tmp_path in the calling tests and build the database value from it instead of f"/tmp/slayer-cache-{n}.db".
• tests/test_engine_factory.py#L411-L413: replace "/tmp/slayer-invalidate.db" with a path derived from tmp_path.
• tests/test_engine_factory.py#L439-L441: replace "/tmp/slayer-reset.db" with a path derived from tmp_path.
• tests/test_engine_factory.py#L457-L459: replace f"/tmp/slayer-conc-{n}.db" with a path derived from tmp_path.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@tests/test_engine_factory.py` around lines 287 - 289, Update the datasource
helpers and their calling tests in tests/test_engine_factory.py at lines
287-289, 411-413, 439-441, and 457-459 to use the pytest tmp_path fixture when
constructing SQLite database paths. Pass tmp_path into the relevant
tests/helpers and derive each database filename from it, replacing all fixed
/tmp paths while preserving the existing unique filenames and test behavior.

Source: Linters/SAST tools

slayer/sql/engine_factory.py (1)

299-314: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Consider bypassing insertion when the cap is 0.

With SLAYER_MAX_CACHED_ENGINES=0, the second lock block inserts the new engine and _take_evictions_over_limit() immediately pops it. Line 312 then disposes the same engine that Line 314 returns. dispose() swaps in a fresh pool, so the returned engine still works, but the insert/evict/dispose cycle is pure overhead on every call.

An early check of the cap before insertion removes that cycle and makes "caching disabled" explicit.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@slayer/sql/engine_factory.py` around lines 299 - 314, Update the cache
insertion block around _engine_cache and _take_evictions_over_limit to bypass
insertion and eviction when the configured maximum cached-engine cap is 0.
Return the newly built engine directly in that case, avoiding disposal of the
same engine being returned; preserve the existing concurrent-winner and eviction
behavior for positive caps.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes